feat: bounded message window for centered Jump to Message#7388
feat: bounded message window for centered Jump to Message#7388diegolmello wants to merge 11 commits into
Conversation
WalkthroughThis PR implements anchored, upper-bounded message windows ( ChangesBounded Message Window for Jump-to-Message
SDK TOTP Argument Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
diegolmello
changed the title
…calls sdk.methodCall appended `this.code || ''` (the TOTP code) as a trailing positional argument to every DDP method call. That empty string was a harmless ignored extra until the server's loadSurroundingMessages method grew a typed `showThreadMessages: boolean` 3rd param (RocketChat/Rocket.Chat #39092). The '' then fails the server's `check(showThreadMessages, Boolean)`, returning a 500 that rejects loadSurroundingMessages and aborts the whole jump-to-message flow (loading shows, no navigation). Append the TOTP code only when a 2FA flow is in progress, so non-2FA DDP calls send exactly their declared params — matching the REST path and the server default. The 2FA flow is preserved: the codeless first call still gets totp-required, then the retry appends the resolved code. Covered by the existing .maestro/tests/room/jump-to-message.yaml flow.
… guard scroll-to-index retry NATIVE-1229 bounded-window jump validation checkpoint. - useMessages: on Anchored Window RELEASE, grow the Live Window by the count of cached messages above the old bound (count += aboveCount) so a deep jump target is not evicted by take(count) and the view no longer snaps to the Live Tail. - useScroll: defer one frame + cap the onScrollToIndexFailed retry to break the synchronous VirtualizedList re-fire recursion (stack overflow) on an unmeasurable target; re-read the target at fire time so a stale retry cannot scroll wrong. - getLocalAnchor / getMessageInfo / services: anchored-jump re-seed wiring. - Regression tests: useMessages (release window growth), useScroll (deferred+capped retry), getLocalAnchor. Includes temporary [JUMP-DBG] instrumentation; to be stripped before merge.
…ntation Strip the in-runtime debug buffer, DB-primitive globals, and the deterministic jump/climb drivers used to validate the bounded-window jump on-device. The real fixes (anchored-window release growth, scroll-to-index retry guard, isMessageInWindow) are unaffected.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
iOS Build Available Rocket.Chat 4.74.0.109044 |
…etry
The onScrollToIndexFailed retry scrolled with only { index, animated: false },
omitting the viewPosition/viewOffset the two primary jump scrolls use to center
the target and clear the sticky room header. On the inverted list this parked the
(already-highlighted) target flush at the top edge, hidden behind the header.
It only surfaced "sometimes" because the retry fires solely when the first
scrollToIndex cannot measure the target's frame yet — distant/mid-window jumps
with no getItemLayout. Near/contiguous jumps took the correctly-offset primary
path.
Consolidate the landing position into one shared JUMP_SCROLL_POSITION constant
applied at all three scrollToIndex sites (initial scroll + retry) so they can no
longer drift apart. Regression test asserts the retry carries the header-clearing
offset.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
iOS Build Available Rocket.Chat 4.74.0.109049 |
diegolmello
had a problem deploying
to
upload_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.74.0.109048 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNT_NRiPGr9JlFGO-oxS3btdz00BqqUy1968eX9Aqax_wZ56RVsJ18G-ZPl0iIiIVd3V1dcmJqGxcgAkk0db |
…hor lifecycle Address code review findings on the bounded-window jump: - Jump completion now grows the Message Window by one page (bounded) when an anchored target sits deeper than the first page, instead of silently waiting for the safety-net abort to fire. Capped at MAX_JUMP_GROWTH_RETRIES. - Guard raiseOrReleaseAnchor against a re-subscribe landing during its awaits (subscription-identity check), so a stale invocation cannot corrupt the new window's count/highTs after a room switch or another raise. - Drop the dead scrollFailRetries reset at the cap so the onScrollToIndexFailed retry budget is truly per-jump (reset only at jump start).
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/views/RoomView/List/hooks/useScroll.test.tsx (1)
19-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMock
fetchMessageswith the same async contract as production.
useScrollexpectsfetchMessages: () => Promise<void>, but this helper injects a plainjest.fn()that returnsundefined. That means this suite won't catch promise-handling regressions in the new growth path. Make the default mock resolve a promise so the tests exercise the real contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/RoomView/List/hooks/useScroll.test.tsx` around lines 19 - 38, The test helper renderUseScroll currently injects fetchMessages as jest.fn() which returns undefined; update the default mock to match the production contract (a function returning a Promise<void>) so tests exercise async behavior—e.g. change the default fetchMessages parameter in renderUseScroll to an async-resolving mock (use fetchMessages = jest.fn().mockResolvedValue(undefined) or an async () => Promise.resolve()) and ensure useScroll is invoked with that mock; keep references to renderUseScroll, useScroll, makeListRef and makeMessagesIdsRef so the helper remains otherwise unchanged.app/views/RoomView/List/hooks/useScroll.ts (2)
72-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve the in-flight jump during unmount cleanup.
This cleanup clears
pendingJump.current?.safety, but it never settles the promise returned byjumpToMessage(). If the list unmounts mid-jump (for example during a room switch), the only remaining resolution path is removed here and any caller awaiting that jump stays pending indefinitely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/RoomView/List/hooks/useScroll.ts` around lines 72 - 80, The cleanup in useEffect currently clears highlightTimeout.current and pendingJump.current.safety but doesn't settle the promise created by jumpToMessage(), leaving callers awaiting indefinitely; update the unmount cleanup in useScroll.ts to, after clearing pendingJump.current.safety, also call the stored completion callback on pendingJump.current (e.g., pendingJump.current.reject or pendingJump.current.resolve) with a cancellation/error value so the jumpToMessage promise is settled, then null out pendingJump.current to avoid leaks; reference pendingJump, jumpToMessage, highlightTimeout and the useEffect cleanup when making the change.
176-182:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBind deferred scroll retries to the jump that scheduled them.
This timeout keeps
params.highestMeasuredFrameIndexfrom the old failure, but it re-reads the latest target id at fire time. If another jump starts before the timer runs and its target is still absent,targetIndexstays-1and this falls back to the previous jump's measured index, which can scroll the new window to the wrong row. Track/cancel the retry timer per jump before issuing the fallback scroll.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/RoomView/List/hooks/useScroll.ts` around lines 176 - 182, The deferred scroll retry must be bound to the specific jump that scheduled it: when scheduling the setTimeout inside useScroll, capture the current jump identifier (e.g., pendingJump.current?.messageId or an internal jumpId) and store the timer id on that jump (e.g., pendingJump.current.retryTimer or a map keyed by jumpId); when a new jump starts clear any existing retry timer for the previous jump; inside the timeout callback verify the captured jumpId still matches the active jump (e.g., pendingJump.current?.messageId === capturedJumpId) before falling back to params.highestMeasuredFrameIndex and calling listRef.current.scrollToIndex, and clear the stored retryTimer on success or cancellation so retries don’t apply to later jumps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/views/RoomView/List/hooks/useScroll.ts`:
- Around line 149-154: The code calls fetchMessages() fire-and-forget inside the
anchored-jump growth branch which can produce unhandled promise rejections; wrap
the fetchMessages() call with explicit error handling (e.g.
fetchMessages().catch(err => { /* surface error and abort the jump */ })) and on
error either abort the jump (use the existing jump state handler or call an
abort function) or set a visible failure state so the safety timeout doesn't
silently wait; ensure you reference and update jumpGrowthRetries.current and
MAX_JUMP_GROWTH_RETRIES logic consistently when handling the error so
retries/aborts remain correct.
---
Outside diff comments:
In `@app/views/RoomView/List/hooks/useScroll.test.tsx`:
- Around line 19-38: The test helper renderUseScroll currently injects
fetchMessages as jest.fn() which returns undefined; update the default mock to
match the production contract (a function returning a Promise<void>) so tests
exercise async behavior—e.g. change the default fetchMessages parameter in
renderUseScroll to an async-resolving mock (use fetchMessages =
jest.fn().mockResolvedValue(undefined) or an async () => Promise.resolve()) and
ensure useScroll is invoked with that mock; keep references to renderUseScroll,
useScroll, makeListRef and makeMessagesIdsRef so the helper remains otherwise
unchanged.
In `@app/views/RoomView/List/hooks/useScroll.ts`:
- Around line 72-80: The cleanup in useEffect currently clears
highlightTimeout.current and pendingJump.current.safety but doesn't settle the
promise created by jumpToMessage(), leaving callers awaiting indefinitely;
update the unmount cleanup in useScroll.ts to, after clearing
pendingJump.current.safety, also call the stored completion callback on
pendingJump.current (e.g., pendingJump.current.reject or
pendingJump.current.resolve) with a cancellation/error value so the
jumpToMessage promise is settled, then null out pendingJump.current to avoid
leaks; reference pendingJump, jumpToMessage, highlightTimeout and the useEffect
cleanup when making the change.
- Around line 176-182: The deferred scroll retry must be bound to the specific
jump that scheduled it: when scheduling the setTimeout inside useScroll, capture
the current jump identifier (e.g., pendingJump.current?.messageId or an internal
jumpId) and store the timer id on that jump (e.g.,
pendingJump.current.retryTimer or a map keyed by jumpId); when a new jump starts
clear any existing retry timer for the previous jump; inside the timeout
callback verify the captured jumpId still matches the active jump (e.g.,
pendingJump.current?.messageId === capturedJumpId) before falling back to
params.highestMeasuredFrameIndex and calling listRef.current.scrollToIndex, and
clear the stored retryTimer on success or cancellation so retries don’t apply to
later jumps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 436f179d-7006-45ba-b668-0297ccf794b2
📒 Files selected for processing (4)
app/views/RoomView/List/hooks/useMessages.tsapp/views/RoomView/List/hooks/useScroll.test.tsxapp/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/views/RoomView/List/index.tsx
- app/views/RoomView/List/hooks/useMessages.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useScroll.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useScroll.test.tsx
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useScroll.test.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useScroll.test.tsx
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place screen components in 'app/views/' directory
Files:
app/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useScroll.test.tsx
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/views/RoomView/List/hooks/useScroll.tsapp/views/RoomView/List/hooks/useScroll.test.tsx
| // Anchored target deeper than the current window: grow by one page (bounded) to pull it in. | ||
| // The safety net still aborts if it never materialises after the cap. | ||
| if (jump.anchored && jumpGrowthRetries.current < MAX_JUMP_GROWTH_RETRIES) { | ||
| jumpGrowthRetries.current += 1; | ||
| fetchMessages(); | ||
| } |
There was a problem hiding this comment.
Handle window-growth fetch failures explicitly.
fetchMessages() is fire-and-forget here. If it rejects, this creates an unhandled rejection and the jump just sits until the five-second safety timeout. Please attach explicit error handling here and either abort the jump or surface the failure immediately.
Based on learnings, this repo avoids floating promises without explicit .catch(...); as per coding guidelines, async operations should use explicit error handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/views/RoomView/List/hooks/useScroll.ts` around lines 149 - 154, The code
calls fetchMessages() fire-and-forget inside the anchored-jump growth branch
which can produce unhandled promise rejections; wrap the fetchMessages() call
with explicit error handling (e.g. fetchMessages().catch(err => { /* surface
error and abort the jump */ })) and on error either abort the jump (use the
existing jump state handler or call an abort function) or set a visible failure
state so the safety timeout doesn't silently wait; ensure you reference and
update jumpGrowthRetries.current and MAX_JUMP_GROWTH_RETRIES logic consistently
when handling the error so retries/aborts remain correct.
Sources: Coding guidelines, Learnings
|
iOS Build Available Rocket.Chat 4.74.0.109053 |
diegolmello
requested a deployment
to
upload_android
GitHub Actions
Waiting
|
Android Build Available Rocket.Chat 4.74.0.109052 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNS7uZ4AhZ-xkYmUPeDNWCAF2nJVGxhh7KhvTZ76CZ-k05LgHg7EWL62vRhDuJ6wGVwsBm79DYavexy0IjcV |
1 participant
Proposed changes
Rebuilds Jump to Message on a bounded message window so a deep jump re-anchors in one step (O(1)) instead of growing the window page-by-page (O(pages)) — removing the root cause of the old 5s-race flakiness — without migrating off the custom native inverted list.
The whole capability is one optional upper
tsbound (highTs) on the existing WatermelonDB observation:highTs == null→ Live Window (newest-first, follows the Live Tail) — unchanged default behavior.highTs→ Anchored Window pinned around the target, below the Live Tail.A pure, DB/React-free anchor resolver decides the bound (
anchorForTarget) and climbs it back toward live as Newer Loaders are consumed (raiseOrRelease), releasing to a Live Window only once the Gap to the Live Tail has fully closed (invariant: never release across an open Gap). "Rejoin live" is now explicit — the Load Newer chain or the jump-to-bottom FAB (which stays visible while anchored).Also included:
take(count)and the list doesn't snap to the Live Tail.onScrollToIndexFailed(nogetItemLayout→ the inverted list re-fires it synchronously → stack overflow). The retry re-applies the same centering + header-clearing view offset as the initial scroll, so a distant jump that falls back to the retry path still lands centered instead of hidden behind the room header.isMessageInWindow), not message origin, so a locally-cached-but-out-of-window target no longer silently aborts; three roads (in-window / cached-out-of-window / server) plus a target-ts fallback.loadSurroundingMessagescall; one-shot jump param so re-searching the same message re-fires; guard the anchor raise/release against a re-subscribe landing mid-flight (room switch / concurrent raise) so a stale read can't corrupt the new window.Design rationale and the rejected alternative (FlashList v2 migration) are in the ADR at
app/views/RoomView/docs/adr/0001-bounded-window-over-flashlist.md. New domain vocabulary is inUBIQUITOUS_LANGUAGE.md(§ Message Loading).Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1224
How to test or reproduce
Screenshots
No static UI redesign; changes are behavioral (centered jump; FAB visibility while anchored).
Types of changes
Checklist
Further comments
The list engine is unchanged; a future FlashList migration remains possible but is neither required nor blocked by this work. Ordering stays
ts-only (ats + _idcomposite is a deferred follow-up noted in the ADR). New unit coverage:anchorResolver,useMessages(release-window growth),useScroll(deferred + capped retry, header-clearing offset on retry, bounded growth + growth cap for deep anchored targets),getLocalAnchor.Summary by CodeRabbit
New Features
Documentation
Tests